-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend locateNodes with a locator to get navigable's container element #811
base: main
Are you sure you want to change the base?
Conversation
b6e0478
to
0a2b364
Compare
@@ -2960,6 +2960,7 @@ To <dfn>await a navigation</dfn> given |navigable|, |request|, |wait condition|, | |||
browsingContext.Locator = ( | |||
browsingContext.AccessibilityLocator / | |||
browsingContext.CssLocator / | |||
browsingContext.ContainerLocator / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we cannot have the container as part of the return value of the browsingContext.getTree
command? It would be somewhat similar to the clientWindow
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that API currently does not include any shared references from script and would not allow expressing the ownership of the returned DOM element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: we could probably plumb it via the getTree along with the other options we might need to define how to return the element but to me it looks like locateNodes is a better place compared to the getTree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a slight misconception here: sharedId
for nodes doesn't depend on ownership. locateNodes
also doesn't provide a way to set the ownership. I broadly think that's fine for this API; it doesn't seem more useful to hold a reference to the container element than the Window itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, when I was reviewing the locateNodes PR I recall there was the ownership parameter but indeed I do not see it after checking now. If we were to use the locateNodes for things other than a11y in Puppeteer, we would definitely need the ownership param to match the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I recall now: so in CDP we retain the resulting collection and not nodes individually so that is still different to the API here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebDriver model to this point is that the sharedId for a node is just an intrinsic property of the node, and returning it doesn't affect the lifetime of the node in any way. executeScript
and friends also provide a way to take (shared) ownership of the (currently only root) return value.
We already have lots of APIs that return objects without providing any way to take ownership of them (notably anything returning a context). I don't immediately see why you'd need to take ownership of an iframe element with this kind of API; if something removes the iframe from the DOM you're probably in an unexpected state even if you still have a reference to the js object.
Anyway, I don't have a really strong opinion here, but if clients ~always want to know which iframe owns a nested browsing context, I think that getTree
would work. If it's a rarer requirement, or there really is a strong use case for taking ownership of the iframe itself, then using either a separate command or a locator could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify on the ownership: all element handles in Puppeteer (and maybe other clients) are retained until the client releases them. It allows getting the a handle to something and, for example, checking that it was disconnected for DOM or operating on temporary objects without polluting the global scope (e.g., creating some elements, retaining them via Puppeteer, using them in scripts later). The getter to obtain the container for a browsing context is the same kind of handle and should follow the same lifecycle.
Currently, we can evaluate using a sharedId to retain but it would be great to avoid the round trip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we currently only have requirements to provide this data in a separate command because this data is not always needed. Therefore, I think we should not bring it into getTree to avoid the overhead of finding out the sharedId for each browsing context. I still think the locateNodes is probably the best place given that it returns nodes and any changes made to locateNodes (e.g., bringing the ownership parameter back) should also affect this method.
b7dc97d
to
0748244
Compare
0748244
to
628c9e0
Compare
@@ -4043,6 +4063,18 @@ The [=remote end steps=] with |session| and |command parameters| are: | |||
1. Let |result nodes| be [=locate nodes using accessibility attributes=] | |||
given |context nodes|, |selector|, and |maximum returned node count|. | |||
|
|||
<dt>|type| is the string "<code>container</code>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not locating by container, we're locating a container by context id. So I think this would make sense if we had the type here as "context" or "containedContext" or something.
<dt>|type| is the string "<code>container</code>" | ||
<dd> | ||
|
||
1. If |start nodes parameter| is not null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this, and the confusion over whether the navigable needs to be a child of the target navigable, points slightly in the direction of a new command being preferable here, but I won't block on that or anything :)
|
||
1. Let |selector| be |locator|["<code>value</code>"]. | ||
|
||
1. Let |child navigable| be |selector|["<code>context</code>"]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a navigable, it's a context id.
|
||
1. Let |returned nodes| be an empty [=/list=]. | ||
|
||
1. If |navigable|'s [=navigable/container=] is not null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the navigable here isn't actually a child of the navigable the command was sent to? That should return an error I assume.
This change adds a new locator called
container
: when called for a parent navigable, given a child navigable as a locator, it locates the container element for the child navigable that is located within the parent navigable. While providing the parent navigable is redundant and it can be computed based on the child navigable, the interface matches existing locators which all select a node within the parent navigable that matches the locator criteria. In this case, the criterion is the child navigable identifier.Providing start nodes to this locator would currently trigger an invalid argument error but eventually it could be supported if there is a use case.
Closes #794
Preview | Diff